Rust: Generalize some models #20652
Rust: Generalize some models #20652
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR generalizes existing Rust models by replacing specific type implementations with trait models, improving data flow detection. The generalization transforms models like <specific_type as trait>::method to <_ as trait>::method, allowing the models to apply to all types implementing the trait rather than just specific implementations.
Key changes:
- Generalized trait models in stdlib framework definitions
- Updated test expectations to reflect improved data flow detection
- Added change note documenting the improvement
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| rust/ql/test/library-tests/dataflow/strings/main.rs | Updated comment to reflect newly detected taint flow |
| rust/ql/test/library-tests/dataflow/strings/inline-taint-flow.expected | Updated test expectations with new generalized models and additional flow detection |
| rust/ql/test/library-tests/dataflow/sources/InlineFlow.expected | Updated test expectations reflecting model generalizations and renumbering |
| rust/ql/test/library-tests/dataflow/local/main.rs | Updated comments to reflect newly detected taint flows |
| rust/ql/lib/codeql/rust/frameworks/stdlib/lang-core.model.yml | Generalized iterator and conversion trait models |
| rust/ql/lib/codeql/rust/frameworks/stdlib/lang-alloc.model.yml | Generalized string, arithmetic, and allocator trait models |
| rust/ql/lib/codeql/rust/frameworks/stdlib/io.model.yml | Generalized IO trait models |
| rust/ql/lib/change-notes/2025-10-15-models.md | Added change note documenting the improvement |
|
DCA LGTM. |
| sink(d); // $ MISSING: hasTaintFlow=90 - we are not currently able to resolve the `parse` call above | ||
| sink_string(b); // $ hasTaintFlow=90 | ||
| sink(c); // $ hasTaintFlow=90 | ||
| sink(d); // $ hasTaintFlow=90 |
| - ["<alloc::string::String as core::ops::arith::Add>::add", "Argument[0].Reference", "ReturnValue", "taint", "manual"] | ||
| - ["<_ as core::ops::arith::Add>::add", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<_ as core::ops::arith::Add>::add", "Argument[0]", "ReturnValue", "taint", "manual"] | ||
| - ["<_ as core::ops::arith::Add>::add", "Argument[0].Reference", "ReturnValue", "taint", "manual"] |
There was a problem hiding this comment.
Yes - its needed for this line in the dataflow/strings test:
sink("Hello ".to_string() + &s1); // $ hasTaintFlow=37
i.e. its a workaround for dealing with the implicit dereference on the RHS when the + is appending strings.
There was a problem hiding this comment.
I wonder if we should have made the model more explicit then, i.e. alloc::string::String as core::ops::arith::Add, since the implementation of + uses a &str as the RHS type instead of the default Self: https://doc.rust-lang.org/std/string/struct.String.html#impl-Add%3C%26str%3E-for-String.
There was a problem hiding this comment.
I'd rather keep it general I think - I don't think there's anything stopping people implementing it that way for other types and if they do, we'll cover that.
3 participants
Generalize some existing models to trait models.
<a as b>::cwith<_ as b>::c, unless there's a good reason why that model only applies toa.